Skip to content

round-trip pvalues without exploding #1463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 24, 2020
Merged

round-trip pvalues without exploding #1463

merged 1 commit into from
Jan 24, 2020

Conversation

dlowe
Copy link
Contributor

@dlowe dlowe commented Dec 30, 2019

When upgrading a codebase from echo v3 to v4, I bumped into this: calling SetParamValues doesn't protect against resizingpvalues, which causes subsequent calls to Reset to panic if it becomes smaller than *e.maxParam.

It's entirely possible this codebase is doing something it shouldn't, to end up in this state (specifically where *e.maxParam != len(c.pvalues)?) Regardless, I believe SetParamValues shouldn't allow callers to screw up the internals of the context.

@dlowe
Copy link
Contributor Author

dlowe commented Jan 2, 2020

@vishr please let me know if I need to do anything else, here. I'm eager to get this fixed so that we can continue migrating to v4 :)

@dlowe
Copy link
Contributor Author

dlowe commented Jan 21, 2020

Ping! I'm still hoping to get someone's attention on this...

@vishr
Copy link
Member

vishr commented Jan 21, 2020

@dlowe Give me few hours :)

@vishr
Copy link
Member

vishr commented Jan 24, 2020

@dlowe Can you look at the failing build?

@dlowe dlowe force-pushed the roundtrip-paramvalues-without-exploding branch from 2f48f23 to 748dc63 Compare January 24, 2020 01:21
@dlowe
Copy link
Contributor Author

dlowe commented Jan 24, 2020

@vishr oooops! Fixed now and rebased.

@codecov
Copy link

codecov bot commented Jan 24, 2020 •
edited
Loading

Codecov Report

Merging #1463 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1463      +/-   ##
==========================================
+ Coverage   84.34%   84.35%   +<.01%     
==========================================
  Files          27       27              
  Lines        2076     2077       +1     
==========================================
+ Hits         1751     1752       +1     
  Misses        212      212              
  Partials      113      113
Impacted Files Coverage Δ
context.go 91.16% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf6888...748dc63. Read the comment docs.

@vishr vishr merged commit 8d7f05e into labstack:master Jan 24, 2020
@dlowe
Copy link
Contributor Author

dlowe commented Jan 24, 2020

@vishr could you please cut a 4.1.14 release soon?

@dlowe dlowe deleted the roundtrip-paramvalues-without-exploding branch January 24, 2020 16:19
@vishr
Copy link
Member

vishr commented Jan 24, 2020

@vishr could you please cut a 4.1.14 release soon?

Done!

@stevenvegt stevenvegt mentioned this pull request Jan 28, 2020
Closed
@pratikmallya pratikmallya mentioned this pull request Feb 2, 2020
Closed
3 tasks
@pratikmallya
Copy link

pratikmallya commented Feb 2, 2020 •
edited
Loading

@dlowe / @vishr how is maxParam supposed to be set? since its a private variable, it can't be set by a config option, and I don't see it being explicitly set anywhere except in the tests.

I am unable to see the use of this parameter. Currently, this change is breaking unit tests in projects that use echo; see: #1492

@dlowe
Copy link
Contributor Author

dlowe commented Feb 2, 2020

@dlowe / @vishr how is maxParam supposed to be set? since its a private variable, it can't be set by a config option, and I don't see it being explicitly set anywhere except in the tests.

Normally, maxParam is set internally by Router.insert(). In tests outside the echo package, this means the only way (currently) to set it is to Add() at least one route, which is what I did here: https://github.com/dlowe/echo/blob/748dc637e2e89b7ac611be30dfbd22e595cbacc9/middleware/jwt_test.go#L64.

I don't think this is ideal... but I don't understand the point of maxParam, so I don't know what would be an appropriate improvement :/

@178inaba 178inaba mentioned this pull request Mar 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishr vishr vishr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dlowe @vishr @pratikmallya